-
-
Notifications
You must be signed in to change notification settings - Fork 17
test: setup E2E testing via Playwright and implement some tests #88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for eslint-code-explorer ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Thanks for getting this started. I don't have time to dig in right now. I'm also not super familiar with this kind of setup, so could use some advice from the rest of the team. @eslint/eslint-team |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 51 out of 52 changed files in this pull request and generated no comments.
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (2)
src/components/ui/popover.tsx:27
- [nitpick] Verify that the use of role 'dialog' on PopoverContent is semantically correct for a popover component; if the component is used to display menus or tips rather than modal dialogs, a different role may be more appropriate.
role="dialog"
src/components/tree-entry.tsx:109
- Ensure that the li element is nested within a ul or ol element to maintain proper HTML semantics.
<li className="flex items-center gap-3">
|
I'm familiar with such setup, will review this pr |
|
@snitin315 friendly ping to not forget about this |
snitin315
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkerschbaum Are we not running Docker in CI? If that's the case, it kind of defeats the purpose of using Docker, since the CI environment would always be inconsistent.
It is always - locally and in CI - running via Docker. |
|
@pkerschbaum can you take a look at the merge conflicts? |
|
I tried running this locally and got this error: > [email protected] test:e2e
> cross-env PW_TEST_CONNECT_WS_ENDPOINT=ws://127.0.0.1:3000/ playwright test
[WebServer] Unable to find image 'mcr.microsoft.com/playwright:v1.51.1-noble' locally
[WebServer] v1.51.1-noble:
[WebServer] Pulling from playwright
[WebServer] 107a4fb0af38: Pulling fs layer
[WebServer] 8c401718c984: Pulling fs layer
[WebServer] 39224aba52ce: Pulling fs layer
[WebServer] 08d1cdd3adad: Pulling fs layer
[WebServer] 08d1cdd3adad: Waiting
[WebServer] 39224aba52ce:
[WebServer] Verifying Checksum
[WebServer] 39224aba52ce: Download complete
[WebServer] 107a4fb0af38:
[WebServer] Verifying Checksum
[WebServer] 107a4fb0af38: Download complete
[WebServer] 8c401718c984: Download complete
[WebServer] 107a4fb0af38: Pull complete
[WebServer] 8c401718c984:
[WebServer] Pull complete
[WebServer] 39224aba52ce:
[WebServer] Pull complete
Error: Timed out waiting 60000ms from config.webServer.
To open last HTML report run:
npx playwright show-reportI'm running Windows 11. |
cd10bf7 to
2e1eeb9
Compare
2e1eeb9 to
69dca82
Compare
can you plz try again? I could reproduce the issue, sometimes the Docker pull takes a long time. |
|
No luck. |
|
Ping @pkerschbaum |
I just noticed that the error is |
|
Same result on a clean try: |
|
I assume you have Docker Desktop in use, so could you please try this: go to settings --> Resources --> Network --> make sure "Enable host networking" is checked. |
|
I agree that an E2E testing setup which does not run easily on every development machine is bad as it makes it difficult / annoying to contribute to the project.
|
|
I think whether or not we want screenshots is a fundamental question to this effort. So the question back to you: what is it you were hoping to catch with screenshots and is there another way to catch those problems? |
Co-authored-by: Percy Ma <[email protected]>
…laywright to latest version
|
I investigated this topic again: I looked at other options for local visual testing (Loki, BackstopJS) but they also depend on Docker (for the same reason of consistent screenshots), so they would not solve the issue we have (brittle Docker setup). So I changed the E2E testing setup to a simple Playwright setup: no Docker anymore, also no screenshot assertions anymore. |
lumirlumir
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Playwright is now working well in my Windows 11 development environment. If other team members have time to check, it would be nice to verify it across different environments.
I've also left a few additional comments throughout the codebase.
lumirlumir
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for all your hard work on this!
This looks like a good start toward adding E2E tests.
Would like @nzakas to verify before merging.
|
When I run it locally, only Chromium succeeds. Everything else times out. However, given that it appears to be running in CI and @lumirlumir indicates it has worked, I think that's good enough to plug it in to cover ourselves. Worst case scenario, we can encourage people to just use Chromium locally if they run into problems. |
Prerequisites checklist
What is the purpose of this pull request?
Introduces End-to-End (E2E) testing using Playwright to prevent regressions and increase confidence during development and refactoring.
What changes did you make? (Give an overview)
Is there anything you'd like reviewers to focus on?
Notes on using Playwright compared to other options
expectAPIs of Playwright are just great; Playwright produces a great report which is expecially useful in CI, ...)Successful CI run: https://github.com/eslint/code-explorer/actions/runs/20301038365/
Note: The initial version of this PR created a Docker setup for consistent screenshot assertions, but that was too fragile - see discussion in this PR.
Archived notes:
Details
The new requirement, having Docker installed, is quite heavy... Thing is that otherwise, screenshots created by screenshot assertions ([`toHaveScreenshot`](https://playwright.dev/docs/api/class-pageassertions#page-assertions-to-have-screenshot-1)) are not consistent, leading to flaky behavior. As [noted by Playwright](https://playwright.dev/docs/test-snapshots#introduction):Alternatives to Docker considered: